Assure breakthrough time is always cast to ms - #14207
Conversation
achaikou
added
release-notes:bug-fix
There was a problem hiding this comment.
Pull request overview
This PR fixes a storage-read failure for breakthrough responses by ensuring the time column is always stored with a consistent Polars datetime unit (ms), even when breakthrough time is None for a realization. This aligns breakthrough response frames across realizations so they can be concatenated/loaded without schema conflicts.
Changes:
- Ensure breakthrough
timeseries is always cast toDatetime('ms'), including the “all None” case. - Introduce an explicit
response_schema()for breakthrough responses and assert it before returning derived frames. - Add a regression test that saves/loads breakthrough responses across realizations with and without a breakthrough time.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ert/config/breakthrough_config.py |
Forces time to ms and asserts a stable schema for derived breakthrough responses. |
tests/ert/unit_tests/config/test_breakthrough_config.py |
Regression test covering stacking/loading breakthrough frames when a realization has None breakthrough time. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14207 +/- ##
=======================================
Coverage 91.93% 91.94%
=======================================
Files 482 484 +2
Lines 33516 33612 +96
=======================================
+ Hits 30813 30903 +90
- Misses 2703 2709 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ajaust
left a comment
ajaust
left a comment
There was a problem hiding this comment.
Regarding the backporting. I assume backporting to the latest release should be fine (like you are doing by tagging the PR). If we realise that we need to backport the fix even further, we can add more labels later (afaik even after merging) and this will create backport PRs.
ajaust
left a comment
ajaust
left a comment
There was a problem hiding this comment.
I just forgot to add the comment about the test assertion to the initial review. 👴
Reproduces and fixes equinor#13962
d8d9ebe to
e1528d0
Compare
|
Successfully created backport PR for |
Issue
Resolves #13962
Approach
In theory we could fix this also in migration, but I think this issue is not so bad to warrant it.
Could backport to more previous releases, IF I figure out which release I need to backport it to...
git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.